Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Reporter tests #3272

Merged
merged 8 commits into from
Apr 1, 2018
Merged

Refactor Reporter tests #3272

merged 8 commits into from
Apr 1, 2018

Conversation

jmuzsik
Copy link

@jmuzsik jmuzsik commented Mar 8, 2018

Description of the Change

In reference to issue: #3260

I did my best at refactoring the code. Though the part of the test files that was quite obviously capable to become more modular was the runner events. I could make all of those calls from one function, ie:

runner.on = runner.once = function...
// into
runner.on = runner.once = runnerEvent(whatevery necessary parameters);

Otherwise I tried to make variables that I noticed were repeatedly called throughout singular files to be called only one time rather then the continual var something = 'something' over and over.

Alternate Designs

I believe that it can still be worked on. It is not perfect. The runnerEvent function could be more beautiful/intuitive. Some tests can make more sense. BaseReporter tests run but it is not obvious, I have no idea how i'd implement that.

Why should this be in core?

Quite a bit less lines of code for the same functionality.

Benefits

Less intensive use of memory and variable calls, etc. More modular code.

Possible Drawbacks

I had to alter quite a bit of code that a lot of people put lots of time into. There are no errors atm, and I expect there not to be.

Applicable issues

Not that I know of

Open to doing more if there is a recommendation.

@jsf-clabot
Copy link

jsf-clabot commented Mar 8, 2018

CLA assistant check
All committers have signed the CLA.

@boneskull
Copy link
Contributor

great effort, thanks. I'll review this when I get a minute

@boneskull boneskull added qa semver-patch implementation requires increase of "patch" version number; "bug fixes" type: cleanup a refactor labels Mar 8, 2018
@coveralls
Copy link

coveralls commented Mar 16, 2018

Coverage Status

Coverage increased (+0.009%) to 90.041% when pulling e059142 on jMuzsik:refactor_reportertests into d76f490 on mochajs:master.

callback(test);
}
};
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about throwing an exception here to prevent typo or new runStr?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok working on it

callback();
}
};
} else if (runStr === 'pending test' || runStr === 'pass' || runStr === 'fail' || runStr === 'end' || runStr === 'suite' || runStr === 'suite end' || runStr === 'test end') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's hard to read and makes a verbose diff when we add a new case.
What do you think about format it like:

} else if (
  runStr === "pending test" ||
  runStr === "pass" ||
  runStr === "fail" ||
  runStr === "end" ||
  runStr === "suite" ||
  runStr === "suite end" ||
  runStr === "test end"
) {

Copy link
Contributor

@boneskull boneskull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, thanks! Awesome work here. I have a few suggestions. Can you take a look?

@@ -3,6 +3,8 @@
var reporters = require('../../').reporters;
var Doc = reporters.Doc;

var runnerEvent = require('./helpers.js').runnerEvent;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what to call this, but runnerEvent isn't descriptive enough.

Also, I'm not sure runner needs to be created as an empty object in beforeEach...

Instead, maybe this could just be a factory function for a runner object? It would then create .on and .once as appropriate.

Then call it something like createMockRunner

*/
function runnerEvent (runStr, ifStr1, ifStr2, ifStr3, arg1, arg2) {
var test = null;
if (runStr === 'start' || runStr === 'pending' || runStr === 'end') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't be afraid to use a switch in here, or an object lookup.


function createElements (argObj) {
var res = [];
for (var i = argObj.from; i <= argObj.to; i += 1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use i++

beforeEach(function () {
stdout = [];
runner = {};
stdoutWrite = process.stdout.write;
process.stdout.write = function (string) {
stdout.push(string);
};

expectedTitle = 'some title';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't make a lot of sense to declare all this above then define it here, does it? Can we not just define it all above?

callback(test);
}
};
// var test = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's this?

@jmuzsik
Copy link
Author

jmuzsik commented Mar 18, 2018

Ok, hope that is an improvement, did just what you said, well I believe I did

@boneskull
Copy link
Contributor

@jmuzsik LGTM! Thanks a ton.

sgilroy pushed a commit to TwineHealth/mocha that referenced this pull request Feb 27, 2019
* Closes mochajs#3260

* refactor-better test harness for reporters tests'

* refactor-better test harness for reporters tests'

* refactor - some additional work

* refactor - helper func

* refactor - helper func

* refactor - helper func

* fix - improvements to helper functions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch implementation requires increase of "patch" version number; "bug fixes" type: cleanup a refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants